Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding FfiType::Callback #1814

Closed
wants to merge 1 commit into from
Closed

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Oct 28, 2023

This is sort-of in the same sprit as #1808, but just focused on foreign callback functions. The goal is to have a single FfiType that we can reuse whenever we want to add a new callback function type.

With this change, if you want to add a new callback function type you just add an item in ffi_callback_definitions(). I think this should make the process much easier. It can help avoid FFI type mismatches, like how I made the Kotlin signature input a i16 instead of an i8.

Use the new type for the future continuation callback. I have another plan for callback interface callbacks. I didn't touch the foreign executor callback, I hope to work on that very soon.

This type can be used whenever we need to define a callback functions.
Rather than adding the type by hand, which was quite annoying when I was
doing the async work, we now can just add an item in
`ffi_callback_definitions()`.

This also can help avoid FFI type mismatches, like how I made the Kotlin
signature input a i16 instead of an i8.

Use the new type for the future continuation callback.  I have another
plan for callback interface callbacks.  I didn't touch the foreign
executor callback, I hope to work on that very soon.
@bendk bendk requested a review from mhammond October 28, 2023 00:07
@bendk bendk requested a review from a team as a code owner October 28, 2023 00:07
@bendk
Copy link
Contributor Author

bendk commented Oct 30, 2023

I thought I was going to need to get some more PRs merged before moving to the second step of this, but I realized I was wrong. I just put up #1818 which includes this change and more. Let's discuss in that PR, I don't think there's much reason to merge half of the changes without the other.

@bendk bendk closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant